Remark storage #10698

arkpar · 2022-01-19T16:27:18Z

This PR Implements https://github.com/paritytech/labs/issues/1

Integrate an IPFS server into Substrate. Populate with all data of remark-events seen. Polkadot-js can then very easily use this to upload data, reference them (via hash) in other transactions, and download via hash through general IPFS. Not good for large files, but great for data < 50kB which would otherwise be difficult to utilise on-chain in an integrated experience for Polkadot-js.

Implementation adds a new runtime function System::remark_with_index. Remark bodies are stored in the TRANSACTION column, much like indexed storage chain content. Serving over IPFS is enabled with --ipfs-server

bkchr · 2022-01-19T20:10:07Z

frame/system/src/lib.rs

+		/// # </weight>
+		#[pallet::weight(T::SystemWeightInfo::remark_with_event(remark.len() as u32))]
+		//#[pallet::weight(T::SystemWeightInfo::remark_with_index(remark.len() as u32))]
+		pub fn remark_with_index(


Do we really want to have this part of frame-system?

This will bleed into any chain using Substrate, including Polkadot/Kusama.

This is not a standalone chain, but a substrate feature indeed. @gavofyork suggested it should be enabled on kusama and eventually polkadot.

Yeah, but that could also be done by a special pallet.

I mean parachains will then also get this functionality etc. IMO we should not enable this by default everywhere. Yes, everybody could filter the call, but that sounds more like a hack to me.

Makes sense. I'll move it to a separate pallet.

stale · 2022-02-19T12:27:39Z

Hey, is anyone still working on this? Due to the inactivity this issue has been automatically marked as stale. It will be closed if no further activity occurs. Thank you for your contributions.

arkpar · 2022-02-19T13:36:37Z

Waiting on #10779

arkpar · 2022-03-15T10:00:47Z

@bkchr This is ready for review again

cheme · 2022-03-15T14:02:28Z

frame/remark/src/lib.rs

+			+ GetDispatchInfo
+			+ From<frame_system::Call<Self>>;
+		/// Weight information for extrinsics in this pallet.
+		type WeightInfo: WeightInfo;


Could also have a

// Origin that control the ability to index a remark. type ControlOrigin: frame_support::traits::EnsureOrigin<Self::Origin>;

but I don't think it is necessary at this point.

cheme

Logic looks good, wonder a bit about the naming.

The PR could call system remark internally, but I don't think it is needed, and I really like this better.
Still, we use a different event, and the pallet could be named differently than pallet-remark (to avoid confusion with system remark). But really depends on the directions that the pallet may take.

One point I find interesting, is that this will only serve content as long as state is not pruned, may need to be extended.
A second point is that sp-io transaction host function are now not only for transaction; and could be renamed (as long as no non test runtime uses them), for instance 'offchain_indexed' instead of 'transaction_index', but maybe offchain is also confusing due to other existing offchain substrate functionalities.
Also been wondering if at some point an api that do not put the ipfs content in block could be interesting (would act like here but with content fetched from ipfs).

(I also added myself a TODO to implement Reference and Release for paritydb (I just realized it is missing))

arkpar · 2022-03-16T16:59:01Z

Logic looks good, wonder a bit about the naming.

The PR could call system remark internally, but I don't think it is needed, and I really like this better. Still, we use a different event, and the pallet could be named differently than pallet-remark (to avoid confusion with system remark). But really depends on the directions that the pallet may take.

This is supposed to be a new version of the remark function. Eventually System::remark should be deprecated.

One point I find interesting, is that this will only serve content as long as state is not pruned, may need to be extended.

That's not correct. Nothing is saved in the state here. Remarks are stored as block bodies basically. They will be kept around unless block body pruning is enabled.

A second point is that sp-io transaction host function are now not only for transaction; and could be renamed (as long as no non test runtime uses them), for instance 'offchain_indexed' instead of 'transaction_index', but maybe offchain is also confusing due to other existing offchain substrate functionalities.

Host function names are not that important. Besides, I think the name is still relevant. transaction_index refers to the fact that a slice of the transaction data is indexed. Which is still true for remarks.

cheme · 2022-03-16T17:05:22Z

That's not correct. Nothing is saved in the state here. Remarks are stored as block bodies basically. They will be kept around unless block body pruning is enabled.

👍 yes, I confused block pruning with state pruning while reading.

bkchr · 2022-03-18T17:03:43Z

frame/remark/src/lib.rs

+		pub fn store(origin: OriginFor<T>, remark: Vec<u8>) -> DispatchResultWithPostInfo {
+			ensure!(!remark.is_empty(), Error::<T>::Empty);
+			let sender = ensure_signed(origin)?;
+			let content_hash = sp_io::hashing::blake2_256(&remark);


Why don't we use the T::Hashing for this?

Okay, I have now seen it is because of the transaction indexing. But that should have maybe been generic as well.

Hashing scheme has to match the scheme supported by the IPFS bitswap server. The generic parameter would need to be propagated to sc-network module somehow. For not it is hardcoded to blake2

frame/remark/src/lib.rs

Co-authored-by: Bastian Köcher <[email protected]>

…remark

stale · 2022-04-18T09:09:28Z

Hey, is anyone still working on this? Due to the inactivity this issue has been automatically marked as stale. It will be closed if no further activity occurs. Thank you for your contributions.

arkpar · 2022-04-18T10:37:21Z

bot merge

paritytech-processbot · 2022-04-18T10:37:28Z

Waiting for commit status.

* Remark storage * Fixed benches * Update frame/remark/src/lib.rs Co-authored-by: Bastian Köcher <[email protected]> * Fixed build * Fixed build Co-authored-by: Bastian Köcher <[email protected]>

arkpar added A0-please_review Pull request needs code review. B7-runtimenoteworthy C1-low PR touches the given topic and has a low impact on builders. D5-nicetohaveaudit ⚠️ PR contains trivial changes to logic that should be properly reviewed. labels Jan 19, 2022

arkpar requested review from bkchr, cheme and shawntabrizi January 19, 2022 16:28

arkpar force-pushed the a-ipfs-remark branch from fc2f3a5 to 2accec8 Compare January 19, 2022 17:54

bkchr reviewed Jan 19, 2022

View reviewed changes

arkpar added A3-in_progress Pull request is in progress. No review needed at this stage. and removed A0-please_review Pull request needs code review. labels Jan 20, 2022

arkpar mentioned this pull request Feb 2, 2022

Refactored block body database scheme #10779

Merged

stale bot added the A5-stale Pull request did not receive any updates in a long time. No review needed at this stage. Close it. label Feb 19, 2022

stale bot removed the A5-stale Pull request did not receive any updates in a long time. No review needed at this stage. Close it. label Feb 19, 2022

arkpar force-pushed the a-ipfs-remark branch 2 times, most recently from 484cce7 to de84ade Compare March 11, 2022 08:38

Remark storage

cabb3f1

arkpar force-pushed the a-ipfs-remark branch from de84ade to cabb3f1 Compare March 11, 2022 08:40

Fixed benches

aaa818c

arkpar added A0-please_review Pull request needs code review. and removed A3-in_progress Pull request is in progress. No review needed at this stage. labels Mar 15, 2022

cheme reviewed Mar 15, 2022

View reviewed changes

cheme approved these changes Mar 15, 2022

View reviewed changes

bkchr approved these changes Mar 18, 2022

View reviewed changes

Update frame/remark/src/lib.rs

e3ec7ad

Co-authored-by: Bastian Köcher <[email protected]>

arkpar added 3 commits March 18, 2022 18:50

Merge branch 'master' of github.com:paritytech/substrate into a-ipfs-…

88b8627

…remark

Fixed build

dbbe384

Fixed build

c92b9bd

stale bot added the A5-stale Pull request did not receive any updates in a long time. No review needed at this stage. Close it. label Apr 18, 2022

stale bot removed the A5-stale Pull request did not receive any updates in a long time. No review needed at this stage. Close it. label Apr 18, 2022

paritytech-processbot bot merged commit 0a7e5ea into master Apr 18, 2022

paritytech-processbot bot deleted the a-ipfs-remark branch April 18, 2022 10:39

wischli mentioned this pull request Apr 22, 2022

feat: upgrade to Polkadot v0.9.19 KILTprotocol/kilt-node#357

Merged

8 tasks

arkpar mentioned this pull request May 3, 2022

Enable remark storage for rococo paritytech/polkadot#5441

Open

github-actions bot mentioned this pull request May 31, 2022

Update substrate/polkadot/cumulus from v0.9.20 to v0.9.22 moonbeam-foundation/moonbeam#1557

Closed

github-actions bot mentioned this pull request Jun 22, 2022

Update substrate/polkadot/cumulus from v0.9.20 to v0.9.23 moonbeam-foundation/moonbeam#1623

Closed

Dengjianping mentioned this pull request Jul 7, 2022

Bump deps to v0.9.22 Manta-Network/Manta#571

Merged

10 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remark storage #10698

Remark storage #10698

arkpar commented Jan 19, 2022 •

edited

Loading

bkchr Jan 19, 2022

arkpar Jan 19, 2022 •

edited

Loading

bkchr Jan 19, 2022

arkpar Jan 19, 2022

stale bot commented Feb 19, 2022

arkpar commented Feb 19, 2022

arkpar commented Mar 15, 2022

cheme Mar 15, 2022

cheme left a comment

arkpar commented Mar 16, 2022

cheme commented Mar 16, 2022

bkchr Mar 18, 2022

bkchr Mar 18, 2022

arkpar Mar 18, 2022

stale bot commented Apr 18, 2022

arkpar commented Apr 18, 2022

paritytech-processbot bot commented Apr 18, 2022

Remark storage #10698

Remark storage #10698

Conversation

arkpar commented Jan 19, 2022 • edited Loading

bkchr Jan 19, 2022

Choose a reason for hiding this comment

arkpar Jan 19, 2022 • edited Loading

Choose a reason for hiding this comment

bkchr Jan 19, 2022

Choose a reason for hiding this comment

arkpar Jan 19, 2022

Choose a reason for hiding this comment

stale bot commented Feb 19, 2022

arkpar commented Feb 19, 2022

arkpar commented Mar 15, 2022

cheme Mar 15, 2022

Choose a reason for hiding this comment

cheme left a comment

Choose a reason for hiding this comment

arkpar commented Mar 16, 2022

cheme commented Mar 16, 2022

bkchr Mar 18, 2022

Choose a reason for hiding this comment

bkchr Mar 18, 2022

Choose a reason for hiding this comment

arkpar Mar 18, 2022

Choose a reason for hiding this comment

stale bot commented Apr 18, 2022

arkpar commented Apr 18, 2022

paritytech-processbot bot commented Apr 18, 2022

arkpar commented Jan 19, 2022 •

edited

Loading

arkpar Jan 19, 2022 •

edited

Loading